[Storage] Use structs for model types where appropriate #7807

ShivangiReja · 2019-09-30T08:28:38Z

Changing the code generation to use structs for model types identified in the API review.

This PR changes the AccountInfo, PageInfo and PageRange models to use struct as per the guidelines .

Here is the gist showing how the generated code would look like: https://gist.github.com/ShivangiReja/ccb498e4fba96452be7a52b156d17966

pakrym · 2019-09-30T18:01:46Z

sdk/storage/Azure.Storage.Blobs/src/Generated/BlobRestClient.cs


                        // Get response headers
                        string _header;
+                        Azure.Core.Http.ETag eTag = default;


Other variables are defined with _ prefix. While I like how they look without prefix we should be consistent.

I'm not actually sure if we want that here. I prefixed the "well known variables" with an _ to avoid conflicts, but these are the sort of values that could cause conflicts with the _ variables. For example, if somebody adds a struct with a property called xml, we wouldn't want that to conflict with our "well known variable" _xml. I think this should probably change back what you originally had.

sdk/storage/Azure.Storage.Blobs/src/Generated/BlobRestClient.cs

pakrym · 2019-09-30T18:07:54Z

sdk/storage/generate.ps1

@@ -10,7 +10,7 @@ autorest --use=$PSScriptRoot/Azure.Storage.Common/swagger/Generator/ --verbose
 cd $PSScriptRoot/Azure.Storage.Queues/swagger/
 autorest --use=$PSScriptRoot/Azure.Storage.Common/swagger/Generator/ --verbose

-cd $PSScriptRoot/Azure.Storage.Files.DataLake/swagger/
-autorest --use=$PSScriptRoot/Azure.Storage.Common/swagger/Generator/ --verbose
+# cd $PSScriptRoot/Azure.Storage.Files.DataLake/swagger/


We can comment this out and check it in. (The Storage folks checked in a work-in-progress that errors out.)

pakrym · 2019-09-30T18:44:27Z

Use the Ordinal/OrginalIgnoreCase comparer for strings.

pakrym · 2019-09-30T18:44:44Z

Figure out if we need to compare byte arrays by value

sdk/storage/Azure.Storage.Blobs/src/Generated/BlobRestClient.cs

AlexanderSher · 2019-09-30T22:19:49Z

sdk/storage/Azure.Storage.Blobs/tests/PageInfoTest.cs

+    public class PageInfoTest
+    {
+        [Test]
+        public void EqualsReturnsTrueForEqualValues()


Are these tests are code-generated?

No, these are hand written tests.

…arison and hash code generation

ShivangiReja · 2019-10-01T06:24:41Z

@tg-msft @pakrym Using StructuralComparisons.StructuralEquality for array comparison and HashCode generation.

We cannot use System.Linq.Enumerable.SequenceEqual because it is not compatible with HashCode generation logic we were using before. My previous hashCode generation was using the default Object.GetHashCode() method which generates HashCode using the memory reference whereas System.Linq.Enumerable.SequenceEqual uses structural equality for comparison.

pakrym · 2019-10-01T14:48:57Z

@tg-msft @pakrym Using StructuralComparisons.StructuralEquality for array comparison and HashCode generation.

This is way better than SequenceEqual good job finding it!

tg-msft · 2019-10-01T14:58:41Z

sdk/storage/Azure.Storage.Blobs/tests/PageInfoTest.cs

+
+namespace Azure.Storage.Blobs.Tests
+{
+    public class PageInfoTest


Maybe add a comment that these are related to our generated struct behavior so they make a little more sense to anyone else finding them in the future.

I agree, I wasn't sure what this test was for at first.

tg-msft · 2019-10-01T14:59:41Z

sdk/storage/Azure.Storage.Common/src/Azure.Storage.Common.csproj

@@ -24,5 +24,6 @@
    -->
    <Compile Include="$(AzureCoreSharedSources)ArrayBufferWriter.cs" Link="AzureCoreShared\%(RecursiveDir)\%(Filename)%(Extension)" />
    <Compile Include="$(AzureCoreSharedSources)ForwardsClientCallsAttribute.cs" Link="AzureCoreShared\%(RecursiveDir)\%(Filename)%(Extension)" />
+    <Compile Include="$(AzureCoreSharedSources)HashCodeBuilder.cs" />


Where does this end up placing the file? Do we want a Link="AzureCoreShared\... like the other included files?

It was working as expected by using <Compile Include="$(AzureCoreSharedSources)HashCodeBuilder.cs" /> only, that's why I didn't realize about adding Link="AzureCoreShared\%(RecursiveDir)\%(Filename)%(Extension)".

I'll update it :)

tg-msft · 2019-10-01T15:37:10Z

sdk/storage/Azure.Storage.Blobs/src/Generated/BlobRestClient.cs


                        // Get response headers
                        string _header;
+                        Azure.Core.Http.ETag eTag = default;


I'm not actually sure if we want that here. I prefixed the "well known variables" with an _ to avoid conflicts, but these are the sort of values that could cause conflicts with the _ variables. For example, if somebody adds a struct with a property called xml, we wouldn't want that to conflict with our "well known variable" _xml. I think this should probably change back what you originally had.

tg-msft

Awesome work Shivangi!

tg-msft · 2019-10-01T15:40:50Z

sdk/storage/Azure.Storage.Common/swagger/Generator/src/generator.ts

@@ -617,6 +626,17 @@ function generateOperation(w: IndentWriter, serviceModel: IServiceModel, group:
                    });
                }
            }
+            if (response.struct) {
+                w.line();
+                const Separator = IndentWriter.createFenceposter();


nit - don't we case locals as separator in TypeScript?

Yes, I got confused between Typescript and .Net. Will update it.

tg-msft · 2019-10-01T15:44:55Z

sdk/storage/Azure.Storage.Common/swagger/Generator/src/generator.ts

+                    w.line(`/// </summary>`);
+                    w.line(`/// <param name="obj">The instance to compare to.</param>`);
+                    w.line(`/// <returns>True if they're equal, false otherwise.</returns>`);
+                    w.line(`public override bool Equals(object obj) => obj is ${naming.type(type.name)} && Equals((${naming.type(type.name)})obj);`);


@pakrym - should we add an [EditorBrowsable(false)] here?

tg-msft · 2019-10-01T15:45:15Z

sdk/storage/Azure.Storage.Common/swagger/Generator/src/generator.ts

+                    w.line(`/// <summary>`)
+                    w.line(`/// Get a hash code for the ${naming.type(type.name)}.`);
+                    w.line(`/// </summary>`);
+                    w.line(`public override int GetHashCode()`);


We should definitely add [EditorBrowsable(false)] here.

sdk/storage/Azure.Storage.Common/swagger/Generator/src/generator.ts

sdk/storage/Azure.Storage.Common/swagger/Generator/src/models.ts

seanmcc-msft · 2019-10-01T17:34:13Z

sdk/storage/Azure.Storage.Blobs/tests/PageInfoTest.cs

+
+namespace Azure.Storage.Blobs.Tests
+{
+    public class PageInfoTest


I agree, I wasn't sure what this test was for at first.

ShivangiReja added 5 commits September 26, 2019 17:11

[Storage] Change AccountInfo class to struct

d41bb32

serialization and deserialization

e284aa4

Add equals and hashcode methods

669d849

Chnage PageInfo class to model

583648c

Change PageRange class to struct

eb47276

ShivangiReja requested review from amnguye, seanmcc-msft and tg-msft as code owners September 30, 2019 08:28

ShivangiReja requested review from pakrym, a-santamaria, AlexGhiondea and KrzysztofCwalina and removed request for a-santamaria September 30, 2019 08:30

Remove unnecessary checks

59447e2

ShivangiReja self-assigned this Sep 30, 2019

ShivangiReja added the Storage Storage Service (Queues, Blobs, Files) label Sep 30, 2019